[Ethereum]: Add support for abi.encodePacked() | Resolves #3679 #3862
Conversation
theghostmac
requested review from
Milerius,
satoshiotomakan and
ar-g
as code owners
There was a problem hiding this comment.
Hi @theghostmac, thank you for opening the PR!
There seems to be protocol mismatches in Uint, Int encoding, could you please take a look?
rust/tw_evm/src/abi/encode.rs
Outdated
| Token::Int { int, .. } => { | ||
| let buf = int.to_big_endian(); | ||
| let trimmed_buf = trim_leading_zeros(buf.as_slice()); | ||
| encoded.extend_from_slice(trimmed_buf); | ||
| }, | ||
| Token::Uint { uint, .. } => { | ||
| let buf = uint.to_big_endian(); | ||
| let trimmed_buf = trim_leading_zeros(buf.as_slice()); | ||
| encoded.extend_from_slice(trimmed_buf); | ||
| }, |
There was a problem hiding this comment.
If I understood the specification correctly, integers are still padded according to the bits parameter.
So for example, uint16 (0x12) should be 0x0012, but this code will produce 0x12 instead
| } | ||
|
|
||
| #[test] | ||
| fn encode_packed_mixed() { |
There was a problem hiding this comment.
Could you please also add this test case https://gist.github.com/miguelmota/bc4304bb21a8f4cc0a37a0f9347b8bbb#file-main-go-L13-L34
There was a problem hiding this comment.
done. does this look good enough?:
#[test]
fn encode_packed_custom_case() {
// bytes32 stateRoots
let state_roots = NonEmptyBytes::from_slice(
"3a53dc4890241dbe03e486e785761577d1c369548f6b09aa38017828dcdf5c27"
.decode_hex()
.unwrap()
.as_slice(),
).unwrap();
// uint256[2] calldata signatures
let signatures = Token::Array {
arr: vec![
Token::Uint {
uint: U256::from_str(
"3402053321874964899321528271743396700217057178612185975187363512030360053932"
).unwrap(),
bits: UintBits::new(256).unwrap(),
},
Token::Uint {
uint: U256::from_str(
"1235124644010117237054094970590473241953434069965207718920579820322861537001"
).unwrap(),
bits: UintBits::new(256).unwrap(),
},
],
kind: ParamType::Uint {
bits: UintBits::new(256).unwrap(),
},
};
// uint256 feeReceivers
let fee_receivers = Token::Uint {
uint: U256::zero(),
bits: UintBits::new(256).unwrap(),
};
// bytes calldata txss
let txss = Token::Bytes(
"000000000000000100010000"
.decode_hex()
.unwrap(),
);
let tokens = vec![Token::FixedBytes(state_roots), signatures, fee_receivers, txss];
let encoded = encode_packed_tokens(&tokens);
let expected = "3a53dc4890241dbe03e486e785761577d1c369548f6b09aa38017828dcdf5c2707857e73108d077c5b7ef89540d6493f70d940f1763a9d34c9d98418a39d28ac02bb0e4743a7d0586711ee3dd6311256579ab7abcd53c9c76f040bfde4d6d6e90000000000000000000000000000000000000000000000000000000000000000000000000000000100010000"
.decode_hex()
.unwrap();
assert_eq!(encoded, expected);
}
```There was a problem hiding this comment.
The code can be cleaned up a bit if you move strings to a separate variable. For example,
let uint1 = "3402053321874964899321528271743396700217057178612185975187363512030360053932";
Token::Uint {
uint: U256::from_str(uint1).unwrap(),
bits: UintBits::new(256).unwrap(),
},Anyway, does this test work?
rust/tw_evm/src/abi/encode.rs
Outdated
| let expected = concat!( | ||
| "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", // -1 | ||
| "42", // 0x42 | ||
| "03", // 3 | ||
| "48656c6c6f2c20776f726c6421" // "Hello, world!" in ASCII | ||
| ).decode_hex().unwrap(); |
There was a problem hiding this comment.
This test should be fixed accordingly
rust/tw_number/src/i256.rs
Outdated
| @@ -19,7 +19,7 @@ lazy_static! { | |||
|
|
|||
| #[derive(Clone, PartialEq)] | |||
| #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] | |||
| pub struct I256(BaseU256); | |||
| pub struct I256(pub BaseU256); | |||
There was a problem hiding this comment.
Consider adding getter/setter functions instead
rust/tw_number/src/u256.rs
Outdated
| @@ -13,7 +13,7 @@ use tw_memory::Data; | |||
|
|
|||
| #[derive(Copy, Clone, Debug, Default, PartialEq)] | |||
| #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] | |||
| pub struct U256(pub(crate) primitive_types::U256); | |||
| pub struct U256(pub primitive_types::U256); | |||
There was a problem hiding this comment.
Add necessary getter/setter functions instead
|
Hi, thanks @satoshiotomakan. I have addressed these. Can you spare time to look at the corrections I made regarding Int and Uint before I open another PR? |
|
@theghostmac why not push changes to the same branch to avoid creating new PRs? |
|
Updated. Everything is in The specific test case you sent from miguelmota, it passes also. |
Description
This pull request adds support for
abi.encodePacked()as described in issue #3679. The implementation follows the non-standard packed mode encoding, which concatenates types shorter than 32 bytes directly without padding or sign extension, encodes dynamic types in-place without the length field, and encodes array elements padded but in-place.How to test
Reviewers can test the changes by running the included test cases that cover various scenarios for
abi.encodePacked()encoding:cargo test encode_packed_tokensencode_packed_addressencode_packed_bytesencode_packed_stringencode_packed_intencode_packed_boolencode_packed_uintencode_packed_fixed_bytesencode_packed_mixedencode_packed_arrayEnsure that all tests pass and verify the encoded outputs match the expected values.
Types of changes
Checklist
If you're adding a new blockchain